Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(NcRichContenteditable): Fix tribute emoji complete interfering unexpectedly aka. 🇨🇨 #3924

Merged
merged 2 commits into from
Mar 24, 2023

Conversation

nickvergessen
Copy link
Contributor

@nickvergessen nickvergessen commented Mar 24, 2023

🪦 Cocos-Island Meme

Bildschirmaufzeichnung vom 2023-03-24, 07-16-01

Opposed to #3923 I fixed it in NcRichContenteditable so that other components like the emoji picker itself are not interfered and still operate normally.

@nickvergessen nickvergessen added bug Something isn't working 3. to review Waiting for reviews papercut Annoying recurring issue with possibly simple fix. feature: rich-contenteditable Related to the rich-contenteditable components labels Mar 24, 2023
@nickvergessen nickvergessen added this to the 7.8.4 milestone Mar 24, 2023
@nickvergessen nickvergessen self-assigned this Mar 24, 2023
Copy link
Contributor

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

…expectedly aka. Cocos Island Meme

Signed-off-by: Joas Schilling <[email protected]>
@nickvergessen nickvergessen force-pushed the bugfix/3825/fix-emoji-autocomplete branch from 28f4500 to 2b371da Compare March 24, 2023 06:49
Copy link
Contributor

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

Copy link
Contributor

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

Copy link
Contributor

@mejo- mejo- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I think that #3923 is the cleaner approach. It doesn't affect the EmojiPicker component either, the search function is only used by emoji autocompletion implementions in NcRichContenteditable and Text/Collectives. That's also why I think that it's the proper place to fix this.

Copy link
Contributor

@mejo- mejo- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this in nevertheless.

@nickvergessen nickvergessen merged commit 43641e0 into master Mar 24, 2023
@nickvergessen nickvergessen deleted the bugfix/3825/fix-emoji-autocomplete branch March 24, 2023 09:00
@nickvergessen
Copy link
Contributor Author

After some discussion in the chat we agreed to go with both approaches in parallel:

  1. This PR here is a UX fix to help users in cases it was not in the intention of the user to autocomplete anything at all but the Enter was planned to submit the input
  2. The PR feat(Emoji): Suggest emojis based on text smiles #3923 is a nice UX improvement to suggest emojis for text smiles so that users can autocomplete them if they want to, but the default behaviour is to keep the impact on users that do NOT wish to do so as low as possible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working feature: rich-contenteditable Related to the rich-contenteditable components papercut Annoying recurring issue with possibly simple fix.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emoji picker suggestions are unexpected when using ":)" and ":D"
4 participants